[CI probe] SOL shader variants on GPU-less runner — do not review#3072
[CI probe] SOL shader variants on GPU-less runner — do not review#3072cptbtptpbcptdtptp wants to merge 9 commits into
Conversation
Two stacked bugs since #1682 left SOL's random-between-two-curves mode entirely inert: an operator-precedence bug in _updateShaderData kept RENDERER_SOL_CURVE_MODE / RENDERER_SOL_IS_RANDOM_TWO from ever being enabled, and the per-particle random factor in instance slot 21 (a_Random0.z) was never written unless the noise module happened to be enabled (#2953 gated the shared slot's write on noise.enabled only). Fix the parenthesization, give SizeOverLifetimeModule its own Rand (using the already-reserved SizeOverLifetime sub-seed), and write the shared slot when SOL needs it; noise keeps precedence when both are on. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
computeParticleSizeMesh was gated on four macros (RENDERER_SOL_CURVE, RENDERER_SOL_RANDOM_CURVES, RENDERER_SOL_CURVE_SEPARATE, RENDERER_SOL_RANDOM_CURVES_SEPARATE) that no TS module ever enables — the random branch even referenced a nonexistent uniform (u_SOLSizeGradientMax) — so the module was entirely inert for mesh-mode particles. Rewrite it against the macro set the billboard path and SizeOverLifetimeModule already use, extended to all three axes. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The mesh-path SOL shader rewrite (3447163) had no automated guard: unit tests only assert TS-side macro/upload/slot state, and no e2e case combined mesh render mode with an enabled sizeOverLifetime module. This case pins computeParticleSizeMesh end-to-end: separateAxes TwoCurves on all three axes with a fixed seed, so per-particle random size variation is baked into the baseline — before the fix every cuboid rendered at constant startSize. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Vitest's coverage collection sweeps e2e/**/*.ts into the report at 0% (they only run under Playwright, never under the unit-test runner), so any e2e-only commit craters codecov's patch metric — this PR's baseline commit dropped it from 97% to 35% with zero source changes. Ignore the directory at the codecov layer; unit coverage semantics are unchanged. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The RENDERER_SOL_IS_SEPARATE + RENDERER_SOL_IS_RANDOM_TWO macro combo hangs SwiftShader's shader compiler (evaluateParticleCurve inlined at 10 call sites in one program), so the case never reached initScreenshot on GPU-less CI runners while passing on real GPUs. Non-separate TwoCurves keeps the mesh path and the per-particle random mix under pixel guard; the separate combo stays covered by unit tests. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
github-actions
Bot
added
the
documentation
WalkthroughAdds randomized size-over-lifetime particle handling, shader and instance-data tests, three mesh-particle E2E cases, their configuration entries, and an E2E coverage exclusion. ChangesParticle size-over-lifetime behavior
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested labels: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
d03fd22 to
8d27be3
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dev/2.0 #3072 +/- ##
===========================================
+ Coverage 79.37% 85.20% +5.82%
===========================================
Files 903 810 -93
Lines 100632 94671 -5961
Branches 11260 11337 +77
===========================================
+ Hits 79879 80663 +784
+ Misses 20569 13918 -6651
+ Partials 184 90 -94
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/src/core/particle/SizeOverLifetime.test.ts`:
- Around line 32-38: Restore the original performance.now implementation after
the mocked timing loop in the test. Capture the original method before
overriding it, then restore it after the engine.update calls in the relevant
test flow, including when the test exits unexpectedly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 2054c6a5-d2f1-45f8-97d4-5d24710e2817
⛔ Files ignored due to path filters (2)
e2e/fixtures/originImage/Particle_particleRenderer-emit-mesh-size-over-lifetime.jpgis excluded by!**/*.jpgpackages/shader/src/ShaderLibrary/Particle/Module/SizeOverLifetime.glslis excluded by!**/*.glsl
📒 Files selected for processing (10)
codecov.ymle2e/case/particleRenderer-emit-mesh-size-over-lifetime.tse2e/case/probe-billboard-sol-separate-two-curves.tse2e/case/probe-billboard-sol-two-curves.tse2e/case/probe-mesh-sol-separate-single-curve.tse2e/case/probe-mesh-sol-single-curve.tse2e/config.tspackages/core/src/particle/ParticleGenerator.tspackages/core/src/particle/modules/SizeOverLifetimeModule.tstests/src/core/particle/SizeOverLifetime.test.ts
| performance.now = function () { | ||
| times++; | ||
| return times * deltaTime; | ||
| }; | ||
| for (let i = 0; i < frames; i++) { | ||
| engine.update(); | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Restore performance.now after mocking to avoid test pollution.
Overwriting the global performance.now without restoring it can leak this mocked state into other test files running in the same environment, potentially causing unrelated time-dependent tests to flake or fail.
🛠️ Proposed fix
+ const originalNow = performance.now;
performance.now = function () {
times++;
return times * deltaTime;
};
for (let i = 0; i < frames; i++) {
engine.update();
}
+ performance.now = originalNow;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| performance.now = function () { | |
| times++; | |
| return times * deltaTime; | |
| }; | |
| for (let i = 0; i < frames; i++) { | |
| engine.update(); | |
| } | |
| const originalNow = performance.now; | |
| performance.now = function () { | |
| times++; | |
| return times * deltaTime; | |
| }; | |
| for (let i = 0; i < frames; i++) { | |
| engine.update(); | |
| } | |
| performance.now = originalNow; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/src/core/particle/SizeOverLifetime.test.ts` around lines 32 - 38,
Restore the original performance.now implementation after the mocked timing loop
in the test. Capture the original method before overriding it, then restore it
after the engine.update calls in the relevant test flow, including when the test
exits unexpectedly.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@e2e/config.ts`:
- Around line 422-426: Update the E2E_CONFIG entry for
Probe3ConebaseInstrumented so it bypasses screenshotWithThreshold and does not
perform baseline image comparison. Route it through a probe-specific test path
or enable the existing explicit skip-comparison behavior while preserving the
probe’s button-visibility validation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: d0191a22-7359-4982-bfec-93e8ddf302b8
📒 Files selected for processing (2)
e2e/case/probe3-conebase-instrumented.tse2e/config.ts
| Probe3ConebaseInstrumented: { | ||
| category: "Particle", | ||
| caseFileName: "probe3-conebase-instrumented", | ||
| threshold: 0, | ||
| diffPercentage: 0.005 |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Keep the baseline-free probe out of screenshot comparison.
E2E_CONFIG routes this case through screenshotWithThreshold, which compares against an origin image after the screenshot button is clicked. Since this probe has no baseline and only button visibility is relevant, it will fail later on missing/invalid comparison data and obscure the compiler result. Run it through a probe-specific test path or add an explicit skip-comparison mode.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@e2e/config.ts` around lines 422 - 426, Update the E2E_CONFIG entry for
Probe3ConebaseInstrumented so it bypasses screenshotWithThreshold and does not
perform baseline image comparison. Route it through a probe-specific test path
or enable the existing explicit skip-comparison behavior while preserving the
probe’s button-visibility validation.
…moved by #3037 on base)
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@e2e/config.ts`:
- Around line 422-433: Update the E2E_CONFIG entries Probe4FinalSeparate and
Probe4FinalNonseparate so they bypass screenshotWithThreshold and do not require
baseline comparison data. Route them through an existing probe-specific path or
enable an explicit skip-comparison mode while preserving the button-visibility
validation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 495b8fec-c227-4337-b5a2-1e369c617dd5
📒 Files selected for processing (3)
e2e/case/probe4-final-nonseparate.tse2e/case/probe4-final-separate.tse2e/config.ts
| Probe4FinalSeparate: { | ||
| category: "Particle", | ||
| caseFileName: "probe4-final-separate", | ||
| threshold: 0, | ||
| diffPercentage: 0.005 | ||
| }, | ||
| Probe4FinalNonseparate: { | ||
| category: "Particle", | ||
| caseFileName: "probe4-final-nonseparate", | ||
| threshold: 0, | ||
| diffPercentage: 0.005 | ||
| }, |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Keep the baseline-free probes out of screenshot comparison.
E2E_CONFIG routes these cases through screenshotWithThreshold, which compares against an origin image after the screenshot button is clicked. Since these probes have no baseline and only button visibility is relevant, they will fail later on missing comparison data and obscure the compiler result. Run them through a probe-specific test path or add an explicit skip-comparison mode.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@e2e/config.ts` around lines 422 - 433, Update the E2E_CONFIG entries
Probe4FinalSeparate and Probe4FinalNonseparate so they bypass
screenshotWithThreshold and do not require baseline comparison data. Route them
through an existing probe-specific path or enable an explicit skip-comparison
mode while preserving the button-visibility validation.
|
Probe complete. Root cause of every hang: this branch's new e2e cases called engine.canvas.resizeByClientSize, which #3037 removed on dev/2.0 (PR CI builds the merge, so old cases — updated by #3037 — passed while newly added ones died in a silent unhandled rejection). Nothing to do with shader variants or SwiftShader. Fix landed on #3060 as 88bc982. |
1 participant
Throwaway probe for #3060: determines which SizeOverLifetime macro variants SwiftShader can compile on GPU-less CI runners (mesh+TwoCurves hangs the compiler; local SwiftShader behaves differently from CI's). The four probe cases have no baselines on purpose — only the 'Screenshot button visible / not visible' log lines matter. Will be closed and deleted after one CI run.
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes